Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rustc --drawing to use Unicode line drawing characters for span output #21406

Closed
wants to merge 2 commits into from

Conversation

kmcallister
Copy link
Contributor

example

I bet we can find other uses for these sorts of characters in clarifying type errors, etc.

This is marked as a "stable" flag for consistency with --color. However we should reconsider both of them as part of #19051. For these features to be conveniently available with Cargo, we may want to use environment variables or a config file.

Keegan McAllister added 2 commits January 19, 2015 13:25
With some other cleanup at the use sites.
…utput

I bet we can find other uses for these sorts of characters in clarifying type
errors, etc.

This is marked as a "stable" flag for consistency with --color. However we
should reconsider both of them as part of rust-lang#19051. For these features to be
conveniently available with Cargo, we may want to use environment variables or
a config file.
@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@sanxiyn
Copy link
Member

sanxiyn commented Jan 20, 2015

This is so cool!

@kmcallister
Copy link
Contributor Author

example 2

@nikomatsakis
Copy link
Contributor

This looks great, I'm just a bit unclear on what the story is regarding adding new flags (as you call out). Have we not even started on finalizing the set of flags yet? Should this be a -Z flag for now?

@alexcrichton
Copy link
Member

I agree with @nikomatsakis that this should probably be unstable for now, you can do so by calling opt::flag_u instead of opt::flag and then invoking rustc via rustc -Z unstable-options --drawing ...

@kmcallister
Copy link
Contributor Author

Shall I mark --color as unstable at the same time?

@alexcrichton
Copy link
Member

For I believe the plan was to consider --color stable.

@kmcallister
Copy link
Contributor Author

In that case, how about an environment variable instead? e.g.

RUST_MESSAGE_FORMAT='color(auto), drawing'

This would not be subject to stability guarantees, on the grounds that it can only affect the formatting of compiler output. Correspondingly, unrecognized syntax in the environment variable would be non-fatal.

Then we could get rid of --color and avoid the question of how it's meant to work with Cargo. I would support stabilizing a command-line flag that disables all "fancy" output formatting, regardless of any environmental or configuration setting.

@alexcrichton
Copy link
Member

I would personally consider "magic environment variables" to be within the realm of stability guarantees we have to worry about. It may not actively break code in this case but we may wish to rename the environment variable or rename drawing at some point in the future which could in theory break compatibility (output-wise, not code-generated-wise).

It is an interesting idea though for something that we don't currently look at much. It's definitely a flexible format that we can ignore old/stale values from.

@kmcallister
Copy link
Contributor Author

We're not generally making stability guarantees about the exact formatting of error messages, are we? I hope we can fix unclear wordings and stuff like that on each 1.x release.

@alexcrichton
Copy link
Member

Yeah I would expect us to be able to reformat error messages at will.

@kmcallister
Copy link
Contributor Author

So I think it's okay for unstable environment variables to affect the output formatting. Although a stable flag to disable it would be good.

@sanxiyn
Copy link
Member

sanxiyn commented Jan 22, 2015

I think we should autodetect Unicode support, just as we do for --color. GCC does so.

@kmcallister
Copy link
Contributor Author

It's not just about Unicode support, but about whether your terminal font includes those characters. I don't know any way to auto-detect that.

@sanxiyn
Copy link
Member

sanxiyn commented Jan 23, 2015

I think autodetection accuracy is less important than a way to workaround when it misbehaves. For example, GCC does misbehave in TERM=linux, but LANG=C gcc is easy enough.

@nikomatsakis
Copy link
Contributor

@brson this might offer a way to make the "downward errors" in your multi-line spans more clearly indicated

@pnkfelix
Copy link
Member

@kmcallister please do not make the control for this solely available via environment variables. I don't mind if you provide that in addition to (unstable) command line options, but I like the option of using just command line options when I can, e.g. in my scripts and what not. Plus they are more self-documenting (in terms of being reported by --help, at least when you do -Z unstable-options --help).

@nikomatsakis
Copy link
Contributor

OK, so it seems like consensus is:

  1. Add an unstable command-line flag for enabling.
  2. Perhaps improve the auto-detection? Certainly if we require that people opt-in to using this, most people won't see the benefit, so more aggressive auto-detection makes some sense to me.
  3. Possibly add an environment variable? I don't have a strong feeling about that.

Do we have a (stable) command-line flag for saying "plain ASCII output please"?

@sanxiyn
Copy link
Member

sanxiyn commented Feb 1, 2015

I propose we restrict ourselves to Windows Glyph List 4 for Unicode characters, in order to have the widest possible font support. It is suboptimal, but in the absence of ways to detect support, the lowest common denominator is the way to go.

Incidentally, that means this patch should use different box drawing characters.

@kmcallister
Copy link
Contributor Author

I can live with WGL4. I'm happy to have at least some basis for thinking these glyphs are widely available.

Perhaps improve the auto-detection?

Let's enable line drawing automatically when we're writing to a terminal and $LANG contains the string UTF-8 by an ASCII-case-insensitive match.

We could also use line drawing when writing to a file. My main concern here is that web servers tend to erroneously serve "plain text" files with a Windows-1252 encoding header.

Do we have a (stable) command-line flag for saying "plain ASCII output please"?

I'll add an unstable one. Then we can bikeshed the name in an RFC. Whatever mechanism we add should integrate with --color, but we can figure out the details later.

@Manishearth
Copy link
Member

Related: should we switch SpanHandler to being a trait and make it interchangeable (plugin)? This way someone could write a structured logging plugin, or a plugin that automatically fixes stuff like unused imports, etc etc.

@kmcallister
Copy link
Contributor Author

What exactly would it take to un-stabilize --color? Should I write an RFC? Is there an existing RFC where it was stabilized?

@Manishearth
Copy link
Member

You should request for color, obviously.

@pnkfelix
Copy link
Member

@kmcallister i don't know if it requires an RFC to destabilize something like --color, I think its more like a thumbs-up from @aturon and @alexcrichton. At the very least, I do not recall there being an RFC when --pretty was moved to -Z unstable-options (see commit 953f294 ).

@alexcrichton
Copy link
Member

I would be fine moving --color behind -Z unstable-options

@kmcallister
Copy link
Contributor Author

New proposal:

  • Identify a set of output capabilities. For present purposes these will be: terminal colors, terminal cursor positioning (see Indicate compilation progress in a form useful to end users #22227), and non-ASCII characters from WGL4, printed as UTF-8. In principle the capabilities are independent, although some combinations may never occur in practice.
  • Each capability has an associated environment variable: RUSTC_OUTPUT_COLOR, RUSTC_OUTPUT_CURSOR_POS, RUSTC_OUTPUT_UTF8. These can be set to always, auto, or never. Any other value will print a warning and act as though the variable was not specified. The default if not specified is auto, which uses heuristics (subject to change) to detect the best choice. These variables are subject to a weak stability guarantee: any value will at worst cause a non-fatal warning in the future. The precise effects on output formatting are not stable, any more than the spelling or formatting of errors is. Warnings about these variables are nonfatal even with -D warnings, i.e. intentionally deny(warnings) doesn't affect all warnings, only those generated through the lint system #21204 behavior. (So maybe they should be notes.)
  • rustc accepts a stable flag --output-plain-ascii. If this flag is specified, or the stable environment variable RUSTC_OUTPUT_PLAIN_ASCII is set to any value, all capabilities are forced to the never state.
  • rustc will do its best to present information in a useful form, using the available capabilities. Of course this is subject to improvement over time. If you think rustc's use of color is confusing, please open a bug before you disable colors :)
  • --color is deprecated, to be removed before 1.0. People with use cases not covered by the new mechanisms are invited to comment on bug #nnnnn.

@kmcallister
Copy link
Contributor Author

@pnkfelix: Does the new proposal satisfy your requirements? There's a command-line flag to disable fancy output, but no flag to enable it, because in my mind this is more of an ambient, per-user UI configuration. Still, we can add such flags if the need becomes clear.

@pnkfelix
Copy link
Member

@kmcallister yeah this is probably fine; I cannot offhand think of a strong reason to push for the flags, other than my own inability to remember the exact shell syntax for setting env variables in various contexts. Thanks for asking.

@kmcallister
Copy link
Contributor Author

I'm not interested in finishing this PR. Let me know if I should close it or what.

If someone else wants to take it over, that's great :)

@Manishearth
Copy link
Member

I might. Maybe. No time now.

@kmcallister
Copy link
Contributor Author

Closing due to inactivity, but see #24387.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants